IR additional health status #2934

End-rey · 2024-09-11T17:43:43Z

Closes #2923.

I'm not sure I've found the right name for the status, and I was also thinking that I could add an additional status before updating contracts, since this is the longest process.
I made a metric type like the storage node (number from enum), but I also thought about the string parameter.

codecov · 2024-09-11T17:47:40Z

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 23.91%. Comparing base (1a5809e) to head (5f97a86).
Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/innerring/state.go	0.00%	2 Missing ⚠️
pkg/metrics/innerring.go	81.81%	2 Missing ⚠️
pkg/innerring/innerring.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2934      +/-   ##
==========================================
+ Coverage   23.88%   23.91%   +0.03%     
==========================================
  Files         775      776       +1     
  Lines       45610    45721     +111     
==========================================
+ Hits        10892    10936      +44     
- Misses      33861    33925      +64     
- Partials      857      860       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/services/control/ir/types.proto

carpawell · 2024-09-12T18:35:33Z

pkg/services/control/ir/types.proto


    // IR application is started and serves all services.
-    READY = 2;
+    READY = 3;


i think you have broken the older version of cli that expect 2 as READY:

neofs-node/cmd/neofs-cli/modules/control/healthcheck.go

Lines 70 to 72 in 4a1bc79

if healthStatus != control.HealthStatus_READY {

os.Exit(1)

}

it is internal API but still i think it is not worth it, @roman-khimov

also, FIY, if we do such things we usually add something to our CHANGELOG to prepare our users for their catastrophic update, e.g.:

neofs-node/CHANGELOG.md

Lines 154 to 170 in 4a1bc79

### Updating from v0.40.1

Remove `notification` section from all SN configuration files: it is no longer

supported. All NATS servers running for this purpose only are no longer needed.

If your app depends on notifications transmitted to NATS, do not update and

create an issue please.

Stop attaching `__NEOFS__NETMAP*` X-headers to NeoFS API requests. If your app

is somehow tied to them, do not update and create an issue please.

Notice that this is the last release containing `blobovnicza-to-peapod`

migration utility. Blobovniczas were removed from the node since 0.39.0, so

if you're using any current NeoFS node version it's not a problem. If you're

using 0.38.0 or lower with blobovniczas configured, please migrate ASAP.

Remove `grpc.tls.use_insecure_crypto` from any storage node configuration.

Remove `timers.emit` from any inner ring configuration.

Compatibility better be kept, it doesn't cost a lot.

Added new statuses after the existing ones, therefore, it no longer breaks.

carpawell · 2024-09-12T18:48:59Z

pkg/innerring/innerring.go

 func New(ctx context.Context, log *zap.Logger, cfg *viper.Viper, errChan chan<- error) (*Server, error) {
 	var err error
 	server := &Server{log: log}

-	server.setHealthStatus(control.HealthStatus_HEALTH_STATUS_UNDEFINED)
+	server.setHealthStatus(control.HealthStatus_CREATE_SERVER)


i think HealthStatus_CREATE_SERVER was not a problem at all. you started a server, it only created &Server{log: log}, so it is really undefined now. i guess the initial issue was mostly about extending codes: you set it like "STARTING_BLOCKCHAIN" and then call server.bc.Run(ctx); you set it like "DEPLOYING_NETWORK" and then you call deploy.Deploy(ctx, deployPrm), etc. it allows understand more about what is happening, starting IR may take minutes now, and all an admin got is minutes of "undefined" that goes immediately to READY then

the rule of thumb may be: any operation that is not about local CPU work but about any I/O that potentially may block and that an admin should know about

Added new statuses that seemed to work with I/O and can block func, but I'm not sure about them.

pkg/services/control/ir/types.proto

cthulhu-rider · 2024-09-17T09:49:00Z

CHANGELOG.md

@@ -5,6 +5,8 @@ Changelog for NeoFS Node

 ### Added
 - More effective FSTree writer for HDDs, new configuration options for it (#2814)
+- New health statuses in inner ring (#2934)


lets add this record with the corresponding commit

Signed-off-by: Andrey Butusov <[email protected]>

New health status for time-spending process - initializing Neo network by blockchain: `INITIALIZING_NETWORK`. Closes #2923. Signed-off-by: Andrey Butusov <[email protected]>

Expose health status of ir via Prometheus. Signed-off-by: Andrey Butusov <[email protected]>

cthulhu-rider

idk why lint action failed

roman-khimov · 2024-09-17T11:55:56Z

idk why lint action failed

Looks a lot like nspcc-dev/neo-go#3416, to be fixed with #2940.

End-rey requested review from roman-khimov, carpawell and cthulhu-rider as code owners September 11, 2024 17:43

End-rey force-pushed the 2923-health_status_undefined branch from d451e1e to f16d6d6 Compare September 11, 2024 17:52

roman-khimov reviewed Sep 11, 2024

View reviewed changes

pkg/services/control/ir/types.proto Outdated Show resolved Hide resolved

carpawell reviewed Sep 12, 2024

View reviewed changes

End-rey force-pushed the 2923-health_status_undefined branch from f16d6d6 to 9acfa84 Compare September 13, 2024 17:08

cthulhu-rider reviewed Sep 16, 2024

View reviewed changes

pkg/services/control/ir/types.proto Outdated Show resolved Hide resolved

End-rey force-pushed the 2923-health_status_undefined branch from 9acfa84 to 9232078 Compare September 16, 2024 14:53

cthulhu-rider reviewed Sep 17, 2024

View reviewed changes

End-rey added 3 commits September 17, 2024 13:04

ir: fix a typo

a7f03d1

Signed-off-by: Andrey Butusov <[email protected]>

ir: add new health status INITIALIZING_NETWORK in ir New func

3149d62

New health status for time-spending process - initializing Neo network by blockchain: `INITIALIZING_NETWORK`. Closes #2923. Signed-off-by: Andrey Butusov <[email protected]>

metrics/ir: add neofs_ir_state_health metric

5f97a86

Expose health status of ir via Prometheus. Signed-off-by: Andrey Butusov <[email protected]>

End-rey force-pushed the 2923-health_status_undefined branch from 9232078 to 5f97a86 Compare September 17, 2024 10:05

cthulhu-rider approved these changes Sep 17, 2024

View reviewed changes

roman-khimov approved these changes Sep 17, 2024

View reviewed changes

roman-khimov merged commit 4f22042 into master Sep 17, 2024
20 of 21 checks passed

roman-khimov deleted the 2923-health_status_undefined branch September 17, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IR additional health status #2934

IR additional health status #2934

End-rey commented Sep 11, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

carpawell Sep 12, 2024

carpawell Sep 12, 2024

roman-khimov Sep 12, 2024

End-rey Sep 13, 2024

carpawell Sep 12, 2024

carpawell Sep 12, 2024

End-rey Sep 13, 2024 •

edited

Loading

cthulhu-rider Sep 17, 2024

cthulhu-rider left a comment •

edited

Loading

roman-khimov commented Sep 17, 2024

	### Updating from v0.40.1
	Remove `notification` section from all SN configuration files: it is no longer
	supported. All NATS servers running for this purpose only are no longer needed.
	If your app depends on notifications transmitted to NATS, do not update and
	create an issue please.

	Stop attaching `__NEOFS__NETMAP*` X-headers to NeoFS API requests. If your app
	is somehow tied to them, do not update and create an issue please.

	Notice that this is the last release containing `blobovnicza-to-peapod`
	migration utility. Blobovniczas were removed from the node since 0.39.0, so
	if you're using any current NeoFS node version it's not a problem. If you're
	using 0.38.0 or lower with blobovniczas configured, please migrate ASAP.

	Remove `grpc.tls.use_insecure_crypto` from any storage node configuration.

	Remove `timers.emit` from any inner ring configuration.

IR additional health status #2934

IR additional health status #2934

Conversation

End-rey commented Sep 11, 2024

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

carpawell Sep 12, 2024

Choose a reason for hiding this comment

carpawell Sep 12, 2024

Choose a reason for hiding this comment

roman-khimov Sep 12, 2024

Choose a reason for hiding this comment

End-rey Sep 13, 2024

Choose a reason for hiding this comment

carpawell Sep 12, 2024

Choose a reason for hiding this comment

carpawell Sep 12, 2024

Choose a reason for hiding this comment

End-rey Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

cthulhu-rider Sep 17, 2024

Choose a reason for hiding this comment

cthulhu-rider left a comment • edited Loading

Choose a reason for hiding this comment

roman-khimov commented Sep 17, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

End-rey Sep 13, 2024 •

edited

Loading

cthulhu-rider left a comment •

edited

Loading